Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix macOS Ruby version handling #9465

Merged
merged 2 commits into from
Dec 8, 2020
Merged

Fix macOS Ruby version handling #9465

merged 2 commits into from
Dec 8, 2020

Conversation

MikeMcQuaid
Copy link
Member

ruby.sh: don't test ruby on macOS. Instead defer to the HOMEBREW_MACOS_SYSTEM_RUBY_NEW_ENOUGH variable.
Also, revert "RbConfig: fix broken MacOS SDK paths" (#9452) because it's a bit more complex and we don't need it any more.

@MikeMcQuaid
Revert "RbConfig: fix broken MacOS SDK paths"
9b4a785 
This reverts commit 11445e0.
@MikeMcQuaid
ruby.sh: don't test ruby on macOS.
f7d72ae 
Instead defer to the `HOMEBREW_MACOS_SYSTEM_RUBY_NEW_ENOUGH`
variable.
@BrewTestBot
Copy link
Member

Review period will end on 2020-12-09 at 17:29:11 UTC.

@BrewTestBot BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Dec 8, 2020
@MikeMcQuaid MikeMcQuaid requested review from mistydemeo and maxim-belkin and removed request for mistydemeo December 8, 2020 17:29
@MikeMcQuaid MikeMcQuaid added the critical Critical change which should be shipped as soon as possible. label Dec 8, 2020
@BrewTestBot BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Dec 8, 2020
@BrewTestBot
Copy link
Member

BrewTestBot commented Dec 8, 2020 •
edited

Review period ended.

@MikeMcQuaid MikeMcQuaid mentioned this pull request Dec 8, 2020
Merged
7 tasks
maxim-belkin
maxim-belkin approved these changes Dec 8, 2020
View reviewed changes
Copy link
Member

@maxim-belkin maxim-belkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this change, unusable_ruby will always return 0 on macOS systems that are not "NEW_ENOUGH", so those systems will be forced to use vendored Ruby. This behaviour is different from what we do on Linux (where we search PATH for usable Ruby)., which is probably OK, so 👍

mistydemeo
mistydemeo approved these changes Dec 8, 2020
View reviewed changes
Copy link
Member

@mistydemeo mistydemeo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Limiting Ruby to "system or Homebrew vendored" on macOS seems entirely reasonable.

@maxim-belkin maxim-belkin merged commit ddfd499 into Homebrew:master Dec 8, 2020
@MikeMcQuaid MikeMcQuaid deleted the ruby-version branch December 9, 2020 10:01
@MikeMcQuaid
Copy link
Member Author

Thanks for approvals and merge @mistydemeo and @maxim-belkin!

With this change, unusable_ruby will always return 0 on macOS systems that are not "NEW_ENOUGH", so those systems will be forced to use vendored Ruby. This behaviour is different from what we do on Linux (where we search PATH for usable Ruby)., which is probably OK, so 👍

Yeh, this was my intent with the recent ruby.sh refactoring but I probably didn't state so explicitly, my bad!

@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Jan 9, 2021
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Jan 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mistydemeo mistydemeo mistydemeo approved these changes

@BrewTestBot BrewTestBot BrewTestBot approved these changes

@maxim-belkin maxim-belkin maxim-belkin approved these changes

Assignees
No one assigned
Labels
critical Critical change which should be shipped as soon as possible. outdated PR was locked due to age
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@MikeMcQuaid @BrewTestBot @mistydemeo @maxim-belkin